test(gossipsub): Topic Membership Tests #1201

shashankshampi · 2024-09-26T06:02:10Z

Test Plan: https://www.notion.so/Gossipsub-651e02d4d7894bb2ac1e4edb55f3192d

Topic Membership Tests:

Test added: (TC6.6: Verify the handling of subscription changes when the underlying pubsub framework sends SUBSCRIBE and UNSUBSCRIBE events.)

Simulate the SUBSCRIBE event and check proper handling in the mesh and gossipsub structures
This test will simulate an UNSUBSCRIBE event and check if the topic is removed from the relevant data structures but remains in gossipsub
This test ensures that multiple topics can be subscribed to and unsubscribed from, with proper initialization of the topic structures.
This test ensures that the number of subscriptions does not exceed the limit set in the GossipSub parameters

Part 2:

TC6.1: Verify that peers can correctly join a topic using JOIN(topic).
TC6.2: Ensure that peers can correctly leave a topic using LEAVE(topic).
TC6.5: Multiple peers join and leave topic simultaneously

diegomrsantos · 2024-09-30T09:16:54Z

how about testgossiptopicmembership.nim?

tests/pubsub/testgossipmembership.nim

diegomrsantos · 2024-09-30T09:52:51Z

We follow conventional commits for the PR title. In this case, it will start with test(gossipsub):

shashankshampi · 2024-10-01T11:30:03Z

how about testgossiptopicmembership.nim?

I am adding other tests in the same file related to Topic Membership Tests for Join topic, leave topic, and mesh updated on peer subscribing and leaving topic.

Earlier suggestion looks fine but else will modify as per your suggestion

shashankshampi · 2024-10-01T11:31:03Z

We follow conventional commits for the PR title. In this case, it will start with test(gossipsub):

Thanks. Will follow same in upcoming PRs

added test wrt subscribe and unsubscribe added tests/pubsub/testgossipinternal2 file linters feat: rendezvous refactor (#1183) Hello! This PR aim to refactor rendezvous code so that it is easier to impl. Waku rdv strategy. The hardcoded min and max TTL were out of range with what we needed and specifying which peers to interact with is also needed since Waku deals with peers on multiple separate shards. I tried to keep the changes to a minimum, specifically I did not change the name of any public procs which result in less than descriptive names in some cases. I also wanted to return results instead of raising exceptions but didn't. Would it be acceptable to do so? Please advise on best practices, thank you. --------- Co-authored-by: Ludovic Chenut <[email protected]> refactor and suite name refactor chore(ci): Enable S3 caching for interop (#1193) - Adds our S3 bucket for caching docker images as Protocol Labs shut down their shared one. - Remove the free disk space workaround that prevented the jobs from failing for using too much space for the images. --------- Co-authored-by: diegomrsantos <[email protected]> PR review comment changes

diegomrsantos · 2024-10-02T11:53:13Z

how about testgossiptopicmembership.nim?

I am adding other tests in the same file related to Topic Membership Tests for Join topic, leave topic, and mesh updated on peer subscribing and leaving topic.

Earlier suggestion looks fine but else will modify as per your suggestion

I'm not sure if gossip membership makes sense.

tests/pubsub/testgossipmembership.nim

tests/pubsub/testgossipmembership

tests/pubsub/testgossipmembership.nim

shashankshampi

addressed review comment

tests/pubsub/testgossipmembership.nim

shashankshampi

All done

tests/pubsub/testgossipmembership.nim

strinnityk · 2024-10-24T12:28:28Z

tests/pubsub/testgossipmembership.nim

+    await gossipSub.switch.stop()
+
+  # Test for verifying peers joining a topic using `JOIN(topic)`
+  asyncTest "handle JOIN topic and mesh is updated":


What's the difference between this case and the handle SUBSCRIBE to the topic one?

The first test is primarily about verifying mesh behavior after joining.
The second test is broader and checks both mesh and gossipsub structures to ensure proper subscription handling.

In short, the second test provides a more thorough validation by checking both the mesh and gossipsub structures, while the first test is specifically focused on the mesh when peers join a topic.

I think you might be able to condense the two cases into one.

Are they two different cases in the test plans?

There were 6 TC on documentation, and I followed the same.
Let me know if this is not required now will remove it

tests/pubsub/testgossipmembership.nim

shashankshampi

Addressed all the review comments as per request.

tests/pubsub/testgossipmembership.nim

shashankshampi · 2024-10-24T13:40:03Z

tests/pubsub/testgossipmembership.nim

+    await gossipSub.switch.stop()
+
+  # Test for verifying peers joining a topic using `JOIN(topic)`
+  asyncTest "handle JOIN topic and mesh is updated":


The first test is primarily about verifying mesh behavior after joining.
The second test is broader and checks both mesh and gossipsub structures to ensure proper subscription handling.

In short, the second test provides a more thorough validation by checking both the mesh and gossipsub structures, while the first test is specifically focused on the mesh when peers join a topic.

tests/pubsub/testgossipmembership.nim

strinnityk

I think it'd be a good addition to this PR to also follow the Given-When-Then formula used in the rest of the codebase. It documents the case by high-level blocks, making it easier to understand for external eyes.

You can check other PRs and or test files for examples.
Also, here's an example using one of your implemented cases:

  asyncTest "handle SUBSCRIBE to the topic":
    # Given 5 gossipsub nodes
    let
      numberOfNodes = 5
      topic = "test-topic"
      nodes = generateNodes(numberOfNodes, gossip = true)

    await allFuturesThrowing(nodes.mapIt(it.switch.start()))

    # When all of them are connected and subscribed to the same topic
    await subscribeNodes(nodes)
    for node in nodes:
      node.subscribe(topic, voidTopicHandler)

    await sleepAsync(2 * DURATION_TIMEOUT)

    # Then their related attributes should reflect that
    for node in nodes:
      let currentGossip = GossipSub(node)

      check currentGossip.topics.contains(topic)
      check currentGossip.gossipsub[topic].len() == numberOfNodes - 1
      check currentGossip.mesh[topic].len() == numberOfNodes - 1

    await allFuturesThrowing(nodes.mapIt(allFutures(it.switch.stop())))

shashankshampi · 2024-10-25T05:43:06Z

asyncTest "handle SUBSCRIBE to the topic":
    # Given 5 gossipsub nodes
    let
      numberOfNodes = 5
      topic = "test-topic"
      nodes = generateNodes(numberOfNodes, gossip = true)

    await allFuturesThrowing(nodes.mapIt(it.switch.start()))

    # When all of them are connected and subscribed to the same topic
    await subscribeNodes(nodes)
    for node in nodes:
      node.subscribe(topic, voidTopicHandler)

    await sleepAsync(2 * DURATION_TIMEOUT)

    # Then their related attributes should reflect that
    for node in nodes:
      let currentGossip = GossipSub(node)

      check currentGossip.topics.contains(topic)
      check currentGossip.gossipsub[topic].len() == numberOfNodes - 1
      check currentGossip.mesh[topic].len() == numberOfNodes - 1

    await allFuturesThrowing(nodes.mapIt(allFutures(it.switch.stop())))

Done. Added documentation as per Given When Then

strinnityk · 2024-10-25T13:08:39Z

tests/pubsub/testgossipmembership.nim

+
+    await allFuturesThrowing(nodes.mapIt(it.switch.start()))
+
+    # When nodes subscribe and then unsubscribe to multiple topics


I'd remove the second part here. Leave only: When nodes subscribe to multiple topics, as that's what you're doing here, and later you're handling the unsubscribe part.

strinnityk · 2024-10-25T13:11:00Z

tests/pubsub/testgossipmembership.nim

+    for topic in topicNames:
+      if gossipSub.topics.len < gossipSub.topicsHigh:
+        gossipSub.subscribe(
+          topic,
+          proc(topic: string, data: seq[byte]): Future[void] {.async.} =
+            discard
+          ,
+        )
+      else:
+        # Then the subscription count should not exceed the limit
+        check gossipSub.topics.len == gossipSub.topicsHigh


Here you don't need to handle the if gossipSub.topics.len < gossipSub.topicsHigh:. That's precisely the situation that we want to test, and that subscribe should be handling:

Suggested change

for topic in topicNames:

if gossipSub.topics.len < gossipSub.topicsHigh:

gossipSub.subscribe(

topic,

proc(topic: string, data: seq[byte]): Future[void] {.async.} =

discard

,

)

else:

# Then the subscription count should not exceed the limit

check gossipSub.topics.len == gossipSub.topicsHigh

for topic in topicNames:

gossipSub.subscribe(

topic,

proc(topic: string, data: seq[byte]): Future[void] {.async.} =

discard

,

)

strinnityk · 2024-10-25T13:13:05Z

tests/pubsub/testgossipmembership.nim

+        # Then the subscription count should not exceed the limit
+        check gossipSub.topics.len == gossipSub.topicsHigh
+
+    check gossipSub.topics.len == gossipSub.topicsHigh


I would check the gossipsub.topics.len against gossipSubParams, which is the limit you defined here. That would make the check accurate.

Also, maybe it'd be interesting to connect the nodes and check for mesh and gossipsub population.

strinnityk · 2024-10-25T13:13:41Z

tests/pubsub/testgossipmembership.nim

+
+    await allFuturesThrowing(nodes.mapIt(it.switch.start()))
+
+    # When nodes join and subscribe to the topic


Isn't join and subscribe the same thing?

tests/pubsub/testgossipmembership.nim

strinnityk · 2024-10-25T13:15:27Z

tests/pubsub/testgossipmembership.nim

+
+    await sleepAsync(2 * DURATION_TIMEOUT)
+
+    # When they all join the topic, their attributes should reflect this


The first half of this should be a bit up, because the connection and subscription is done in the lines above.

strinnityk · 2024-10-25T13:16:32Z

tests/pubsub/testgossipmembership.nim

+      check currentGossip.mesh.hasKey(topic)
+      check currentGossip.topics.contains(topic)
+
+    # When peers subscribe to each other's topics


The subscription is done above, on the subscribe. This is just a synchronous check to turn the test deterministic: Make sure all nodes are connected between themselves.

shashankshampi

Done all as per request

shashankshampi · 2024-10-26T06:02:12Z

tests/pubsub/testgossipmembership.nim

+
+    await allFuturesThrowing(nodes.mapIt(it.switch.start()))
+
+    # When nodes subscribe and then unsubscribe to multiple topics


shashankshampi · 2024-10-26T06:03:46Z

tests/pubsub/testgossipmembership.nim

+    for topic in topicNames:
+      if gossipSub.topics.len < gossipSub.topicsHigh:
+        gossipSub.subscribe(
+          topic,
+          proc(topic: string, data: seq[byte]): Future[void] {.async.} =
+            discard
+          ,
+        )
+      else:
+        # Then the subscription count should not exceed the limit
+        check gossipSub.topics.len == gossipSub.topicsHigh


shashankshampi · 2024-10-26T06:07:26Z

tests/pubsub/testgossipmembership.nim

+        # Then the subscription count should not exceed the limit
+        check gossipSub.topics.len == gossipSub.topicsHigh
+
+    check gossipSub.topics.len == gossipSub.topicsHigh


shashankshampi · 2024-10-26T06:07:57Z

tests/pubsub/testgossipmembership.nim

+
+    await allFuturesThrowing(nodes.mapIt(it.switch.start()))
+
+    # When nodes join and subscribe to the topic


tests/pubsub/testgossipmembership.nim

shashankshampi · 2024-10-26T06:13:18Z

tests/pubsub/testgossipmembership.nim

+
+    await sleepAsync(2 * DURATION_TIMEOUT)
+
+    # When they all join the topic, their attributes should reflect this


shashankshampi · 2024-10-26T06:14:22Z

tests/pubsub/testgossipmembership.nim

+      check currentGossip.mesh.hasKey(topic)
+      check currentGossip.topics.contains(topic)
+
+    # When peers subscribe to each other's topics


strinnityk · 2024-10-28T13:53:22Z

tests/pubsub/testgossipmembership.nim

+      check currentGossip.gossipsub.hasKey(topic)
+      check currentGossip.topics.contains(topic)


Please, do these checks as exhaustive as possible :)

all 3 validations is already there. Please help with any validation you think is missed out

strinnityk · 2024-10-28T13:53:50Z

tests/pubsub/testgossipmembership.nim

+      check currentGossip.gossipsub.hasKey(topic)
+      check currentGossip.mesh.hasKey(topic)
+      check currentGossip.topics.contains(topic)


Same here (re: exhaustiveness).

all three validations are already there. Please help with any validation you think is missed out

shashankshampi added 3 commits September 26, 2024 11:28

added test wrt subscribe and unsubscribe

871efab

added tests/pubsub/testgossipinternal2 file

dc7f8d4

linters

5790b6f

shashankshampi marked this pull request as ready for review September 26, 2024 07:58

shashankshampi requested review from diegomrsantos, fbarbu15, lchenut and strinnityk September 26, 2024 07:59

shashankshampi added 2 commits September 26, 2024 13:33

Merge branch 'master' into block6Test

eced002

refactor and suite name refactor

1c2e221

diegomrsantos reviewed Sep 30, 2024

View reviewed changes

tests/pubsub/testgossipmembership.nim Outdated Show resolved Hide resolved

diegomrsantos reviewed Sep 30, 2024

View reviewed changes

tests/pubsub/testgossipmembership.nim Outdated Show resolved Hide resolved

diegomrsantos mentioned this pull request Sep 30, 2024

test(gossipsub): block5 protobuf test cases #1204

Merged

shashankshampi requested review from AYAHASSAN287 and romanzac October 1, 2024 10:20

shashankshampi closed this Oct 1, 2024

shashankshampi reopened this Oct 1, 2024

shashankshampi added 3 commits October 1, 2024 17:18

test(gossipsub): added test for membership for join and leave topic

2923a2d

test(gossipsub): import optimization

fda0d2b

shashankshampi force-pushed the block6Test branch from 9230ecd to eb2f6bf Compare October 1, 2024 11:59

shashankshampi changed the title ~~Part 1: Test cases covering subscribe and unsubscribe Events~~ test(gossipsub): Part 1 Test cases covering subscribe and unsubscribe Events Oct 1, 2024

shashankshampi added 2 commits October 2, 2024 16:25

Merge branch 'master' into block6Test

9c0966e

Merge branch 'master' into block6Test2

27c2850

diegomrsantos reviewed Oct 2, 2024

View reviewed changes

tests/pubsub/testgossipmembership.nim Outdated Show resolved Hide resolved

review comment fix

d80abe0

strinnityk requested changes Oct 22, 2024

View reviewed changes

shashankshampi added 2 commits October 24, 2024 13:42

rebase

a7e80de

Remove tests/pubsub/testgossipmembership from the PR

be818b9

shashankshampi commented Oct 24, 2024

View reviewed changes

rebase

a3c29ae

shashankshampi requested a review from strinnityk October 24, 2024 08:24

strinnityk requested changes Oct 24, 2024

View reviewed changes

tests/pubsub/testgossipmembership.nim Outdated Show resolved Hide resolved

fix in last test

aa34c7f

shashankshampi commented Oct 24, 2024

View reviewed changes

tests/pubsub/testgossipmembership.nim Outdated Show resolved Hide resolved

strinnityk requested changes Oct 24, 2024

View reviewed changes

another set of review comment fix

df674d5

shashankshampi commented Oct 24, 2024

View reviewed changes

shashankshampi requested a review from strinnityk October 24, 2024 13:47

shashankshampi marked this pull request as ready for review October 24, 2024 13:47

strinnityk reviewed Oct 24, 2024

View reviewed changes

added documentation as per request

1bfdded

shashankshampi requested a review from strinnityk October 25, 2024 05:45

shashankshampi changed the title ~~test(gossipsub): Part 1 Test cases covering subscribe and unsubscribe Events~~ test(gossipsub): Topic Membership Tests Oct 25, 2024

strinnityk reviewed Oct 25, 2024

View reviewed changes

added alex changes

49ce782

shashankshampi commented Oct 26, 2024

View reviewed changes

shashankshampi requested a review from strinnityk October 26, 2024 06:16

strinnityk reviewed Oct 28, 2024

View reviewed changes

shashankshampi requested a review from strinnityk November 5, 2024 11:27

rlve closed this May 12, 2025

github-project-automation bot moved this from backlog to done in nim-libp2p May 12, 2025

rlve mentioned this pull request May 12, 2025

test(gossipsub): Topic Membership Tests - updated #1363

Merged

richard-ramos deleted the block6Test branch August 28, 2025 14:46


		await allFuturesThrowing(nodes.mapIt(it.switch.start()))

		# When nodes subscribe and then unsubscribe to multiple topics


		await allFuturesThrowing(nodes.mapIt(it.switch.start()))

		# When nodes join and subscribe to the topic


		await sleepAsync(2 * DURATION_TIMEOUT)

		# When they all join the topic, their attributes should reflect this

		check currentGossip.gossipsub.hasKey(topic)
		check currentGossip.topics.contains(topic)

test(gossipsub): Topic Membership Tests #1201

test(gossipsub): Topic Membership Tests #1201

Conversation

shashankshampi commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diegomrsantos commented Sep 30, 2024

Uh oh!

Uh oh!

Uh oh!

diegomrsantos commented Sep 30, 2024

Uh oh!

shashankshampi commented Oct 1, 2024

Uh oh!

shashankshampi commented Oct 1, 2024

Uh oh!

diegomrsantos commented Oct 2, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shashankshampi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shashankshampi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shashankshampi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

strinnityk left a comment

Choose a reason for hiding this comment

Uh oh!

shashankshampi commented Oct 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shashankshampi commented Sep 26, 2024 •

edited

Loading